Skip to content

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Jan 14, 2025

This got added in #132482 but the PR does not explain why. @lukas-code do you still remember? Also Cc @Noratrieb as reviewer of that PR.

If I understand the issue description correctly, all paths under which this type is exported are stable now: core::array::TryFromSliceError and std::array::TryFromSliceError. If that is the case, we shouldn't have the attribute; it's a terrible hack that should only be used when needed to maintain backward compatibility. Getting some historic information right is IMO not sufficient justification to risk accidentally exposing this type via more unstable paths today or in the future.

@rustbot
Copy link
Collaborator

rustbot commented Jan 14, 2025

r? @joboet

rustbot has assigned @joboet.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jan 14, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jan 14, 2025

The rustc-dev-guide subtree was changed. If this PR only touches the dev guide consider submitting a PR directly to rust-lang/rustc-dev-guide otherwise thank you for updating the dev guide with your changes.

cc @BoxyUwU, @jieyouxu, @Kobzol

@lukas-code
Copy link
Member

The point of adding this atrribute was to make the "since" version on the rustdoc page show up correctly: https://doc.rust-lang.org/nightly/core/array/struct.TryFromSliceError.html

std::array::TryFromSliceError is stably exposed by that path in 1.34.0, but the module std::array is only stably exposed in 1.35.0. (This is becaue we didn't the full path stability back then.)

Usually, we want to show the stability of the "most unstable" path component in rustdoc, this is important for example for core::error::Error, which itself has a #[stable(since = "1.0.0")] attribute, but shows as "stable since 1.81.0", because that's when core::error got stabilized: https://doc.rust-lang.org/nightly/core/error/trait.Error.html

But by that logic std::array::TryFromSliceError would also show as stable since 1.35.0.


Getting some historic information right is IMO not sufficient justification to risk accidentally exposing this type via more unstable paths today or in the future.

This seems plausible to me, and there are probably very few crates that still use MSRV 1.34.0 and name this type dicectly, so I'm fine with removing this again.

@RalfJung
Copy link
Member Author

RalfJung commented Jan 14, 2025

The point of adding this atrribute was to make the "since" version on the rustdoc page show up correctly: https://doc.rust-lang.org/nightly/core/array/struct.TryFromSliceError.html

It is news to me that rustdoc actually uses the attribute when computing the information shown there, but maybe it does? Cc @notriddle

I am not sure if there is some other way to get rustdoc to show the correct information here, but whatever we do should be rustdoc-only and not affect how rustc behaves on current stable releases. But it's probably not worth worrying about this tiny mistake only affecting a single version released five years ago...

Usually, we want to show the stability of the "most unstable" path component in rustdoc, this is important for example for core::error::Error, which itself has a #[stable(since = "1.0.0")] attribute, but shows as "stable since 1.81.0", because that's when core::error got stabilized: https://doc.rust-lang.org/nightly/core/error/trait.Error.html

TBH I find that page confusing since it is not at all clear that the "Since 1.81.0" refers to the path rather than the item.

@notriddle
Copy link
Contributor

notriddle commented Jan 14, 2025

It is news to me that rustdoc actually uses the attribute when computing the information shown there, but maybe it does?

I don't think the allowed_through_unstable_modules attribute is supposed to affect the version number shown there. That version number is calculated in this code:

https://github.com/rust-lang/rust/blob/master/src/librustdoc/passes/propagate_stability.rs

If the item was inlined, it'll pick the version number from the import, but allowed_through_unstable_modules off the original item. allowed_through_unstable_modules isn't specified on imports because it makes no sense there.

TBH I find that page confusing since it is not at all clear that the "Since 1.81.0" refers to the path rather than the item.

Assuming we want the number there to refer to the item, where should the stability of the path be specified? We don't want someone to copy a path into their code, with a "stable since 1.0" badge on it, only to actually try it in the 1.73 compiler and get rug-pulled when it turns out not to actually be stable. That's pretty bad for people's confidence in the docs.

The clearest, most explicit option of flat-out adding another attribute, like this?

Trait Error

Since 1.0.0 • In core since 1.81.0 • Source

It's unambiguous, but it's also not great for progressive disclosure, because that's a pretty complex concept that most people don't need to care about.

@RalfJung
Copy link
Member Author

RalfJung commented Jan 15, 2025

I don't think the allowed_through_unstable_modules attribute is supposed to affect the version number shown there. That version number is calculated in this code:

Odd. Then why does https://doc.rust-lang.org/nightly/core/array/struct.TryFromSliceError.html show 1.34? It seems to ignore the stability of the path components... but clearly those are not ignored, as demonstrated by core::error::Error.

Assuming we want the number there to refer to the item, where should the stability of the path be specified?

Good question. Maybe it should say explicitly that the item is stable since X, but only available under this particular path since Y. I don't know how to best make that concise though. For the common case where both of these are the same version, we can keep the current display of course, so hopefully the "progressive disclosure" concern is remedied by this fact? The "path stability" flag could also have some hover / long-press text explaining what this is about.

Anyway that's a separate discussion from this PR.

@notriddle
Copy link
Contributor

Odd. Then why does https://doc.rust-lang.org/nightly/core/array/struct.TryFromSliceError.html show 1.34?

You're right. It affects the version number in this case.

if let Some(own_stab) = own_stability
&& let StabilityLevel::Stable { since: own_since, allowed_through_unstable_modules: false } =
own_stab.level
&& let Some(parent_stab) = parent_stability
&& (parent_stab.is_unstable()
|| parent_stab.stable_since().is_some_and(|parent_since| parent_since > own_since))
{
parent_stability

Items inherit stability from their parent module if they don't have this attribute and if their stability version is lower than the parent module's. That includes the version number. My mistake. I was too focused on inlining through use statements, and not thinking about parent modules.

@RalfJung
Copy link
Member Author

r? libs

@rustbot rustbot assigned tgross35 and unassigned joboet Jan 23, 2025
@tgross35
Copy link
Contributor

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Jan 23, 2025

📌 Commit 6103896 has been approved by tgross35

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 23, 2025
@bors bors merged commit a3fb2a0 into rust-lang:master Jan 24, 2025
6 checks passed
@rustbot rustbot added this to the 1.86.0 milestone Jan 24, 2025
@RalfJung RalfJung deleted the TryFromSliceError branch January 27, 2025 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-rustc-dev-guide Area: rustc-dev-guide S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants